Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis change introduces a template formatter plugin for Hologram, enabling formatting of ~HOLO sigil templates and .holo files. The implementation includes a formatter module implementing the Mix.Tasks.Format behavior, an algebra-based document formatter with comprehensive template handling, parser updates for proper text tag emission, configuration registration, and extensive test coverage. Changes
Sequence Diagram(s)sequenceDiagram
participant Mix as Mix.Format
participant Fmt as Formatter
participant Parser
participant Algebra
participant Docs as Inspect.Algebra
Mix->>Fmt: format(content, options)
Fmt->>Fmt: Extract whitespace & content
Fmt->>Parser: parse_markup(content)
Parser-->>Fmt: tokens
Fmt->>Algebra: format_tokens(tokens)
Algebra->>Algebra: Parse & transform nodes
Algebra->>Algebra: Build doc structure
Algebra-->>Fmt: doc
Fmt->>Docs: format(doc, line_length)
Docs-->>Fmt: iodata
Fmt->>Fmt: Convert to binary & preserve newline
Fmt-->>Mix: formatted content
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
This is marked as a draft until more niggles are worked out. Upon request, I wanted to get this out publicly to make status more clear (and encourage myself to get back to work on it.) As of now, applying the formatter in the repository itself changes 17 files. Some of these changes I find agreeable; other changes vary from "not so much" to "breaking." I have also used some AI assistance to get the output to this point. I know that it contains non-idiomatic Elixir in places and I will be performing my own code review. I hope to get it into a state where I can use the tests to avoid regressions in doing so. I have also applied this to my own Hologram projects and found its results useful. I think the average "in the wild" Hologram project will have less edges to navigate, so getting the output right here will be a good enough win for mainline inclusion. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/hologram/template/parser.ex (1)
646-652: Reset the token buffer when entering a raw block.
Line 648 flushes buffered text but doesn’t clear it, so pre-raw text can be re-emitted inside the raw block at Line 657.🐛 Proposed fix
def parse_tokens(%{raw?: false} = context, :text, [{:symbol, "{%raw}"} = token | rest]) do context |> maybe_add_text_tag() + |> reset_token_buffer() |> add_processed_token(token) |> add_processed_tag({:block_start, "raw"}) |> enable_raw_mode() |> parse_tokens(:text, rest) end
🤖 Fix all issues with AI agents
In `@lib/hologram/template/algebra.ex`:
- Around line 446-469: from_attr_value currently treats parts that are entirely
empty/whitespace as absent and returns empty(), which drops explicit
empty-string attributes like id=""; change the logic so that when relevant_parts
== [] you first check the original parts for an explicit {:text, ""} (or any
text exactly equal to ""), and if present return concat(string("="),
string("\"\"")) (use the existing concat/string helpers) instead of empty();
keep the existing flow (using tighten_expression, hd(relevant_parts), etc.) for
other cases so only truly omitted attributes remain boolean while explicit empty
strings are preserved.
- Around line 82-129: The recursive consumers do_consume_until_end_tag/4 and
do_consume_until_end_block/4 lack base clauses for an empty token list, causing
a FunctionClauseError on unbalanced markup; add explicit [] clauses to both
functions to handle unclosed items (e.g. defp do_consume_until_end_tag([],
tag_to_match, _acc, _level) and defp do_consume_until_end_block([],
tag_to_match, _acc, _level)) and return a clear error (for example {:error,
{:unclosed_tag, tag_to_match}} and {:error, {:unclosed_block, tag_to_match}}) so
callers of consume_until_end_tag and consume_until_end_block can handle unclosed
tags/blocks instead of crashing.
| defp consume_until_end_tag(tokens, tag_to_match) do | ||
| do_consume_until_end_tag(tokens, tag_to_match, [], 0) | ||
| end | ||
|
|
||
| defp do_consume_until_end_tag([{:start_tag, {tag, _}} = token | rest], tag_to_match, acc, level) | ||
| when tag == tag_to_match do | ||
| do_consume_until_end_tag(rest, tag_to_match, [token | acc], level + 1) | ||
| end | ||
|
|
||
| defp do_consume_until_end_tag([{:end_tag, tag} = _token | rest], tag_to_match, acc, level) | ||
| when tag == tag_to_match do | ||
| if level == 0 do | ||
| {Enum.reverse(acc), rest} | ||
| else | ||
| do_consume_until_end_tag(rest, tag_to_match, [{:end_tag, tag} | acc], level - 1) | ||
| end | ||
| end | ||
|
|
||
| defp do_consume_until_end_tag([token | rest], tag_to_match, acc, level) do | ||
| do_consume_until_end_tag(rest, tag_to_match, [token | acc], level) | ||
| end | ||
|
|
||
| defp consume_until_end_block(tokens, tag_to_match) do | ||
| do_consume_until_end_block(tokens, tag_to_match, [], 0) | ||
| end | ||
|
|
||
| defp do_consume_until_end_block( | ||
| [{:block_start, {tag, _}} = token | rest], | ||
| tag_to_match, | ||
| acc, | ||
| level | ||
| ) | ||
| when tag == tag_to_match do | ||
| do_consume_until_end_block(rest, tag_to_match, [token | acc], level + 1) | ||
| end | ||
|
|
||
| defp do_consume_until_end_block([{:block_end, tag} = _token | rest], tag_to_match, acc, level) | ||
| when tag == tag_to_match do | ||
| if level == 0 do | ||
| {Enum.reverse(acc), rest} | ||
| else | ||
| do_consume_until_end_block(rest, tag_to_match, [{:block_end, tag} | acc], level - 1) | ||
| end | ||
| end | ||
|
|
||
| defp do_consume_until_end_block([token | rest], tag_to_match, acc, level) do | ||
| do_consume_until_end_block(rest, tag_to_match, [token | acc], level) | ||
| end |
There was a problem hiding this comment.
Add explicit base cases for unclosed tags/blocks.
Without a [] clause, Line 83/105 will crash with FunctionClauseError on unbalanced markup rather than a useful error.
🐛 Proposed fix
defp consume_until_end_tag(tokens, tag_to_match) do
do_consume_until_end_tag(tokens, tag_to_match, [], 0)
end
+defp do_consume_until_end_tag([], tag_to_match, _acc, _level) do
+ raise Hologram.TemplateSyntaxError, message: "Unclosed </#{tag_to_match}> tag."
+end
+
defp do_consume_until_end_tag([{:start_tag, {tag, _}} = token | rest], tag_to_match, acc, level)
when tag == tag_to_match do
do_consume_until_end_tag(rest, tag_to_match, [token | acc], level + 1)
end
@@
defp consume_until_end_block(tokens, tag_to_match) do
do_consume_until_end_block(tokens, tag_to_match, [], 0)
end
+defp do_consume_until_end_block([], tag_to_match, _acc, _level) do
+ raise Hologram.TemplateSyntaxError, message: "Unclosed {/#{tag_to_match}} block."
+end
+
defp do_consume_until_end_block(
[{:block_start, {tag, _}} = token | rest],
tag_to_match,
acc,
level
)🤖 Prompt for AI Agents
In `@lib/hologram/template/algebra.ex` around lines 82 - 129, The recursive
consumers do_consume_until_end_tag/4 and do_consume_until_end_block/4 lack base
clauses for an empty token list, causing a FunctionClauseError on unbalanced
markup; add explicit [] clauses to both functions to handle unclosed items (e.g.
defp do_consume_until_end_tag([], tag_to_match, _acc, _level) and defp
do_consume_until_end_block([], tag_to_match, _acc, _level)) and return a clear
error (for example {:error, {:unclosed_tag, tag_to_match}} and {:error,
{:unclosed_block, tag_to_match}}) so callers of consume_until_end_tag and
consume_until_end_block can handle unclosed tags/blocks instead of crashing.
| defp from_attr_value([]), do: empty() | ||
|
|
||
| defp from_attr_value(parts) when is_list(parts) do | ||
| relevant_parts = | ||
| Enum.filter(parts, fn | ||
| {:text, t} -> String.trim(t) != "" | ||
| {:expression, _} -> true | ||
| end) | ||
|
|
||
| if relevant_parts == [] do | ||
| empty() | ||
| else | ||
| value = | ||
| Enum.map_join(parts, fn | ||
| {:text, t} -> t | ||
| {:expression, e} -> tighten_expression(e) | ||
| end) | ||
|
|
||
| if length(relevant_parts) == 1 and elem(hd(relevant_parts), 0) == :expression do | ||
| concat(string("="), string(value)) | ||
| else | ||
| concat(string("="), string("\"" <> value <> "\"")) | ||
| end | ||
| end |
There was a problem hiding this comment.
Preserve explicit empty-string attribute values.
When value == "" (e.g., id=""), Line 455 returns empty() and outputs a boolean attribute instead of ="".
🐛 Proposed fix
defp from_attr_value(parts) when is_list(parts) do
relevant_parts =
Enum.filter(parts, fn
{:text, t} -> String.trim(t) != ""
{:expression, _} -> true
end)
- if relevant_parts == [] do
- empty()
- else
- value =
- Enum.map_join(parts, fn
- {:text, t} -> t
- {:expression, e} -> tighten_expression(e)
- end)
-
- if length(relevant_parts) == 1 and elem(hd(relevant_parts), 0) == :expression do
- concat(string("="), string(value))
- else
- concat(string("="), string("\"" <> value <> "\""))
- end
- end
+ value =
+ Enum.map_join(parts, fn
+ {:text, t} -> t
+ {:expression, e} -> tighten_expression(e)
+ end)
+
+ if relevant_parts == [] do
+ # preserve explicit empty/whitespace-only values
+ concat(string("="), string("\"" <> value <> "\""))
+ else
+ if length(relevant_parts) == 1 and elem(hd(relevant_parts), 0) == :expression do
+ concat(string("="), string(value))
+ else
+ concat(string("="), string("\"" <> value <> "\""))
+ end
+ end
end🤖 Prompt for AI Agents
In `@lib/hologram/template/algebra.ex` around lines 446 - 469, from_attr_value
currently treats parts that are entirely empty/whitespace as absent and returns
empty(), which drops explicit empty-string attributes like id=""; change the
logic so that when relevant_parts == [] you first check the original parts for
an explicit {:text, ""} (or any text exactly equal to ""), and if present return
concat(string("="), string("\"\"")) (use the existing concat/string helpers)
instead of empty(); keep the existing flow (using tighten_expression,
hd(relevant_parts), etc.) for other cases so only truly omitted attributes
remain boolean while explicit empty strings are preserved.
mwmiller
force-pushed
the
template_formatter
branch
from
9d61c54 to
e10466b
Compare
bartblast
left a comment
bartblast
left a comment
There was a problem hiding this comment.
Thanks for pushing this forward, @mwmiller!
Since this is a draft I'll focus on high-level architectural decisions and formatting approach rather than implementation details.
Architecture
The Algebra module currently does two jobs (from what I understand):
- Building a nested tree from
Parser's flat token list (parse_tokens/1,consume_until_*) - Formatting using
Inspect.Algebra
Worth extracting tree-building into a separate Hologram.Template.DOMTree module:
Parser → flat tokens
DOMTree → nested tree
Formatter → formatted string
This makes the tree structure reusable for other purposes and keeps the formatter focused on rendering.
Inspect.Algebra seems like a reasonable choice - we can evaluate as we go if it gives us the flexibility we need.
Formatting heuristics
We should base our formatting behavior on established tools like Prettier rather than inventing our own rules. The current logic expands too aggressively:
<div>a</div>becomes:
<div>
a
</div>Prettier distinguishes between inline elements (where whitespace affects rendering) and block elements (where it doesn't), and tries to fit content on one line when possible. It's worth studying Prettier's HTML formatting approach before finalizing the heuristics here.
|
PS: The dev branch includes CI improvements that significantly speed up the GitHub Actions workflow and improve the feedback loop. You might want to merge those changes into your branch to benefit from the faster iteration. |
The current tests all pass which means producing the desired output Next up we add some more tests for potentially errant edges to smooth things out
mwmiller
force-pushed
the
template_formatter
branch
from
e10466b to
d733632
Compare
2 participants
This is a second pass at introducing a template formatter for HOLO sigils (and also
.holofiles.) This version uses Inspect Algebra for better overall results.Summary by CodeRabbit
Release Notes
New Features
mix formatsupport for.holofiles and~HOLOsigils with proper handling of inline/block elements, attributes, whitespace normalization, and nested structures.Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.